fix(memory): preserve reusable scope configs - #7068
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughMemory normalization now copies incoming dictionaries before modifying them. Regression tests verify that repeated validation preserves caller-owned configuration, normalized paths, dependencies, and inferred ChangesMemory Input Immutability
Merge Risk: ⚪ Minimal · up to This localized change preserves caller-owned memory configuration mappings without changing validated values or public behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
I’ve synced this branch with the latest |
|
Thanks for the pull request. First-time contributors need an associated open issue before we can review a PR.
See the contributing guide. |
|
I have opened #7204 and linked it in this PR with |
|
The CI runs when you open or reopen a PR request, I think you would have edited post making a PR |
|
Ready to merge after #7200 greens CI (pypdf bump for pip-audit). This change itself looks good; the current red checks are unrelated. |
Vidit-Ostwal
added
the
llm-generated
Vidit-Ostwal
left a comment
Vidit-Ostwal
left a comment
There was a problem hiding this comment.
Confirmed the three mutation paths still reproduce on current main: MemoryScope/MemorySlice pop("memory"), in-place scopes normalization, and _ensure_memory_kind inserting memory_kind into the caller’s mapping. Reusing the same config leaves the second view unbound.
The shallow dict(...) copies before those writes are the right fix, and the three regression tests cover reuse, path normalization, and discriminator inference. Branch is current with main, CI is green, and CodeRabbit has nothing to address.
LGTM.
2 participants
Summary
MemoryScopeandMemorySliceconfiguration dictionaries reusable across validationsProblem
The memory view validators modified their input mappings in place. Constructing a
MemoryScoperemoved the runtimememorydependency from the caller's dictionary, so reusing the same configuration produced a second scope that was not bound to its memory.MemorySlicealso rewrote the caller's scope paths, and legacy discriminator inference insertedmemory_kindinto the original mapping.Implementation
Each validation path now makes a shallow copy immediately before it mutates the mapping. The validated values, normalized paths, serialization format, and public API remain unchanged.
Validation
uv run --python 3.12 pytest -q lib/crewai/tests/memory/: 150 passeduv run --python 3.12 pre-commit run --files lib/crewai/src/crewai/memory/memory_scope.py lib/crewai/tests/memory/test_unified_memory.py: Ruff, formatting, and mypy passedAI assistance
AI assistance was used to inspect the validator mutation paths and draft the regression tests. I reproduced the behavior before the change, reviewed every changed line, and ran the validations above locally.
The required
llm-generatedlabel could not be applied because GitHub does not grant external contributors permission to label upstream pull requests. Please apply it during triage.Fixes #7204